Skip to content

Obsolete Microsoft.AspNetCore.HttpOverrides.IPNetwork #62490

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Draft
wants to merge 19 commits into
base: main
Choose a base branch
from
Draft

Obsolete Microsoft.AspNetCore.HttpOverrides.IPNetwork #62490

wants to merge 19 commits into from
+22 −163

Conversation

WeihanLi
Copy link
Contributor

@WeihanLi WeihanLi commented Jun 27, 2025 •
edited
Loading

Obsolete Microsoft.AspNetCore.HttpOverrides.IPNetwork

  • You've read the Contributor Guide and Code of Conduct.
  • You've included unit or integration tests for your change, where applicable.
  • You've included inline docs for your change, where applicable.
  • There's an open issue for the PR that you are making. If you'd like to propose a new feature or change, please open an issue to discuss the change or find an existing issue.

Obsolete Microsoft.AspNetCore.HttpOverrides.IPNetwork to prefer System.Net.IPNetwork

Description

Mark Microsoft.AspNetCore.HttpOverrides.IPNetwork as obsolete, and prefer System.Net.IPNetwork

fixes #46157

@github-actions github-actions bot added the area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions label Jun 27, 2025
@dotnet-policy-service dotnet-policy-service bot added the community-contribution Indicates that the PR has been added by a community member label Jun 27, 2025
@build-analysis build-analysis bot mentioned this pull request Jun 28, 2025
Open
@MihaZupan
Copy link
Member

I don't think this change should go through until dotnet/runtime#117114 is addressed.
As-is this is bound to break people due to differences in behavior.

@MihaZupan MihaZupan added the blocked The work on this issue is blocked due to some dependency label Jun 30, 2025
@WeihanLi
Copy link
Contributor Author

Yeah, thanks, that's why still makes it as draft

stephentoub
stephentoub reviewed Jul 11, 2025
View reviewed changes
src/Middleware/HttpOverrides/src/ForwardedHeadersOptions.cs Outdated
@@ -83,7 +84,10 @@ public class ForwardedHeadersOptions
/// <summary>
/// Address ranges of known proxies to accept forwarded headers from.
/// </summary>
public IList<IPNetwork> KnownNetworks { get; } = new List<IPNetwork>() { new IPNetwork(IPAddress.Loopback, 8) };
public IList<IPNetwork> KnownNetworks { get; } = new List<IPNetwork>()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change won't be necessary once the runtime is updated, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, I think so, will revert in later update

stephentoub
stephentoub reviewed Jul 11, 2025
View reviewed changes
src/Middleware/HttpOverrides/src/IPNetwork.cs Outdated

return true;
}
public static implicit operator IPNetwork(System.Net.IPNetwork ipNetwork) => new IPNetwork(ipNetwork);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need this? In general we try to stay away from implicit operators that allocate. What does this help?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Trying to improve the experience when using a System.Net.IPNetwork, since it's going be removed, maybe the value is pretty low, let me remove this

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is so users of ForwardedHeadersOptions.KnownNetworks don't break when updating their apps.

Although, I think regardless this would be a binary breaking change because they'd still need to recompile to get the implicit operator? In which case, it's probably better to just not have the implicit operator and force them to use the new type.

stephentoub
stephentoub reviewed Jul 11, 2025
View reviewed changes
src/Middleware/HttpOverrides/test/IPNetworkTest.cs Outdated

namespace Microsoft.AspNetCore.HttpOverrides;

public class IPNetworkTest
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If it's being used, even if obsoleted, we should continue to have tests for it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, will revert in later update

@build-analysis build-analysis bot mentioned this pull request Jul 11, 2025
Open
@halter73
Copy link
Member

I agree we should obsolete Microsoft.AspNetCore.HttpOverrides.IPNetwork type to avoid confusion, but we may never actually delete it. I think we can keep any methods used convert from the old type to System.Net.IPNetwork for the purposes of backwards compatibility internal. It's not like HttpOverrides.IPNetwork was an exchange type, and it's very similar to the new type, so it should be pretty easy to migrate code to use a new System.Net.IPNetwork-property without needing us to provide public conversion methods.

The big question is what to name the new System.Net-based version of the ForwardedHeadersOptions.KnownNetworks property now that the good name has been burned by the to-be-obsoleted type. We cannot simply change the type and retain binary compatibility with an API we've shipped since at least .NET Core 2.

How does KnownIPNetworks sound? We could update #46157 to be an API proposal issue with this name and mark it api-ready-for-review. Using the [Obsolete] attribute on the existing KnownNetworks property can point people to the right popertiy to use.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@stephentoub stephentoub stephentoub left review comments

@BrennanConroy BrennanConroy BrennanConroy left review comments

Copilot code review Copilot Awaiting requested review from Copilot Copilot will automatically review once the pull request is marked ready for review

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions blocked The work on this issue is blocked due to some dependency community-contribution Indicates that the PR has been added by a community member
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Obsolete IPNetwork
5 participants
@WeihanLi @MihaZupan @halter73 @stephentoub @BrennanConroy